-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[release/3.1] Context Menus are sometimes not shown in High-DPI applications #2098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…8 in), a change was made to `Popup` that involved the destruction and recreation of the underlying `HWND`. This was done to ensure that the `HWND` was always created with the correct monitor (~=DPI) affinity. In order to acheive this, `DestroyWindow()` and `BuildWindow()` - private methods in `Popup` - were used. We are finding that `DestroyWindow()` has side-effects that can lead to incorrect behavior. The incorrect beahvior works as follows: - The call into `Popup.CreateWindow` is usually a consequence `Popup.IsOpenChanged` (`false -> true`) - Within `Popup.CreateWindow`, `DestroyWindow()` is called (when high-dpi mode is detected). - `DestroyWindow()` destroys the underlying `HWND`, releases the capture, raises the *OnClosed* event and clears the placement-target. - At the end of `DestroyWindow()`, we get `IsOpen == false`. - After `DestroyWindow()`, we call into `BuildWindow()` and `CreateNewPopupRoot()` etc., which go on to build the `Popup` again (and also instnatiate a new `HWND`). - Unfortunately, there is no mechanism here for resetting `IsOpen` back to `true` (without also leading to an undesirable infinite-recursion that calls back into `CreateWindow`). If these calls to show the context-menu arise from `ContextMenu.OnIsOpenChanged`, and flow through `ContextMenu.HookupParentPopup -> Popup.CreateRootPopup`, then `IsOpen` gets reset (there is a direct call into `SetBinding(IsOpenProperty)`, in `Popup.CreateRootPopupInternal`) and the popup is shown correctly. Until then, the context-menu is "stuck" not being able to be shown. The solution is to stop using `DestroyWindow()` as-is, which does more than what we need for it to accomplish. Our original intent in calling `DestroyWindow()` was simply destroy and recreate the `HWND`. This fix refactors `DestroyWindow()` to suit this need and uses the newly introduced `DestroyWindowImpl()` to destroy the `HWND`, and then recreate just that. The rest of the state is retained intact, and the `Popup/ContextMenu` continues to function well as before.
ghost
requested a review
from rladuca
October 23, 2019 22:16
ghost
requested a review
from SamBent
October 23, 2019 22:16
vatsan-madhavan
added
* NO MERGE *
metadata: The PR is not ready for merge yet (see discussion for detailed reasons)
issue-type-netfx-port
Ports from .NET Framework
regression
status: This issue is a regression from a previous build or release
labels
Oct 23, 2019
arpitmathur
approved these changes
Oct 24, 2019
rladuca
approved these changes
Oct 24, 2019
vatsan-madhavan
added
auto_merge
bot-command
and removed
* NO MERGE *
metadata: The PR is not ready for merge yet (see discussion for detailed reasons)
labels
Oct 29, 2019
Hello @vatsan-madhavan! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Is there any way to mitigate this until the next version of .NET Framework? |
ghost
locked as resolved and limited conversation to collaborators
Apr 14, 2022
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
auto_merge
bot-command
issue-type-netfx-port
Ports from .NET Framework
PR
metadata: Label to tag PRs, to facilitate with triage
regression
status: This issue is a regression from a previous build or release
Servicing-approved
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #2088
.NET 5 PR: #2097
Description (Summary)
As part of a previous fix (that shipped originally as part of .NET 4.8 in), a change was made to
Popup
that involved the destruction and recreation of the underlyingHWND
. This was done to ensure that theHWND
was always created with the correct monitor (~=DPI) affinity.This previous fix depended upon a private helper-method (
Popup.DestroyWindow
) that had side-effects that were not accounted for in the original fix. One of the side effects is that theContextMenu
is not sown consistently.The solution modifies (refactors) the private method to extract the useful portion and uses it to improve the previous fix.
Customer Impact
This is a forward-port from from .NET 4.8. This was reported by a customer, and has also been discovered by Visual Studio internally.
Regression
Regression introduced by .NET 4.8. .NET Core 3.0 shipped with this bug.
Risk
Low - this has been well tested internally and validated by multiple customers. The fix is well understood and small/scoped.
Details
As part of a previous fix (that shipped originally as part of .NET 4.8 in), a change was made to
Popup
that involved the destruction and recreation of the underlyingHWND
. This was done to ensure that theHWND
was always created with the correct monitor (~=DPI) affinity.In order to acheive this,
DestroyWindow()
andBuildWindow()
- private methods inPopup
- were used.We are finding that
DestroyWindow()
has side-effects that can lead to incorrect behavior. The incorrect beahvior works as follows:Popup.CreateWindow
is usually a consequencePopup.IsOpenChanged
(false -> true
)Popup.CreateWindow
,DestroyWindow()
is called (when high-dpi mode is detected).DestroyWindow()
destroys the underlyingHWND
, releases the capture, raises the OnClosed event and clears the placement-target.DestroyWindow()
, we getIsOpen == false
.DestroyWindow()
, we call intoBuildWindow()
andCreateNewPopupRoot()
etc., which go on to build thePopup
again (and also instnatiate a newHWND
).IsOpen
back totrue
(without also leading to an undesirable infinite-recursion that calls back intoCreateWindow
).If these calls to show the context-menu arise from
ContextMenu.OnIsOpenChanged
, and flow throughContextMenu.HookupParentPopup -> Popup.CreateRootPopup
, thenIsOpen
gets reset (there is a direct call intoSetBinding(IsOpenProperty)
, inPopup.CreateRootPopupInternal
) and the popup is shown correctly. Until then, the context-menu is "stuck" not being able to be shown.The solution is to stop using
DestroyWindow()
as-is, which does more than what we need for it to accomplish. Our original intent in callingDestroyWindow()
was simply destroy and recreate theHWND
. This fix refactorsDestroyWindow()
to suit this need and uses the newly introducedDestroyWindowImpl()
to destroy theHWND
, and then recreate just that. The rest of the state is retained intact, and thePopup/ContextMenu
continues to function well as before.